Skip to content

Conversation

lumynou5
Copy link
Contributor

@lumynou5 lumynou5 commented Mar 7, 2025

Originally, the indentation of the scripts was not unified: ones in 'scripts/' used 2 spaces, and ones in '.ci/' used 4. Even though the former used 2 spaces, the style was not implemented well as some indents in the files were 4 spaces.

This patch unifies the style of the scripts, and note it with an EditorConfig file. EditorConfig is used because it's supported out-of-the-box by many editors, helping the auto-indent functionality works.

Change-Id: I41be8779ef5b0e94192c144bb48605625727963f

@lumynou5
Copy link
Contributor Author

lumynou5 commented Mar 7, 2025

Is it needed to separate re-formatting files and introducing EditorConfig into two commits?

Copy link
Contributor

@jserv jserv left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mention EditorConfig in top-level documentation (i.e., README.md).

@jserv
Copy link
Contributor

jserv commented Mar 8, 2025

Is it needed to separate re-formatting files and introducing EditorConfig into two commits?

Always minimize the necessary changes.

Copy link
Contributor

@jserv jserv left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rebase the latest master branch.

Originally, the indentation of the scripts in directory 'scripts/' and
the GitHub workflow was inconsistent due to human error, and some files
contains lines with only spaces.

This patch introduces EditorConfig, which is supported out-of-the-box by
many editors, to help the auto-indent functionality work. It sets the
indentation to the currently used indentation of each file respectively,
and ask editors to use Unix-like newlines (line feed at the end of each
line) and avoid trailing white-spaces.

Change-Id: Idaa878cb779b637c1361baa637419645828530cb
@jserv
Copy link
Contributor

jserv commented Mar 10, 2025

Is it possible to enforce the style checks in GitHub Actions as well?

@lumynou5
Copy link
Contributor Author

lumynou5 commented Mar 10, 2025

Is it possible to enforce the style checks in GitHub Actions as well?

There are some tools such as editorconfig-check, but I think the simplest way is to run a headless editor and diff the content before and after re-indenting, for example:

cp $file $file.bak
nvim --headless +'norm vG=' +'x' $file
diff $file $file.bak

However, the current scripts might not pass the test because it seems unable to handle "aligning" like

VAR = $(command --flag arg |
        another --flag arg)
^ These space characters let the commands aligned.

@jserv
Copy link
Contributor

jserv commented Mar 10, 2025

There are some tools such as editorconfig-check, but I think the simplest way is to run a headless editor and diff the content before and after re-indenting.

It sounds great. Create an issue to enforce consistent style for the shell scripts after the merging of this pull request.

@jserv jserv merged commit f4b4bc2 into sysprog21:master Mar 10, 2025
1 of 2 checks passed
@jserv
Copy link
Contributor

jserv commented Mar 10, 2025

Thank @lumynou5 for contributing!

@lumynou5
Copy link
Contributor Author

lumynou5 commented Mar 10, 2025

Also, EditorConfig is not language-aware. It doesn't know that Bash here document must not be indented with spaces (or not be indented at all). Therefore, rather than enforcing checks, I recommend to let it just help editors find correct indentation and avoid errors like before this PR, and human developers can align or do whatever making it more readable to humans.

@lumynou5 lumynou5 deleted the editorconfig branch March 10, 2025 15:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants